Update next_funding spec requirements#4098
Conversation
The next_funding TLV is included in channel_reestablish when either tx_signatures or commitment_signed is needed for an interactive-tx session. This commit largely includes the spec requirements in the comments, including when to retransmit commitment_signed. But it also adds an additional check that tx_signatures has not yet been received.
|
👋 Thanks for assigning @wpaulino as a reviewer! |
|
It's unclear to me if and where we'd need to change the implementation to reflect the following spec change: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4098 +/- ##
==========================================
+ Coverage 87.81% 88.72% +0.90%
==========================================
Files 176 176
Lines 131770 132779 +1009
Branches 131770 132779 +1009
==========================================
+ Hits 115719 117804 +2085
+ Misses 13416 12290 -1126
- Partials 2635 2685 +50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
| // - if `next_commitment_number` is 1 in both the `channel_reestablish` it | ||
| // sent and received: | ||
| // - MUST retransmit `channel_ready`. |
There was a problem hiding this comment.
While testing, I noticed there's an overlap between this requirement and my_current_funding_locked when splicing. If we open a fresh channel and immediately splice it, we'll retransmit channel_ready after a reconnect since the commitments have not advanced even though we already locked the splice (either explicitly via splice_locked or implicitly via my_current_funding_locked).
There was a problem hiding this comment.
Mmm, probably worth skipping the channel_ready if we have a splice indicator in the reestablish.
There was a problem hiding this comment.
We can probably leave this for a follow-up? Relevant discussion in the spec: lightning/bolts#1160 (comment)
There was a problem hiding this comment.
Sure, will land this then, its otherwise trivial.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM, modulo @wpaulino's comment.
| // - if `next_commitment_number` is 1 in both the `channel_reestablish` it | ||
| // sent and received: | ||
| // - MUST retransmit `channel_ready`. |
There was a problem hiding this comment.
Mmm, probably worth skipping the channel_ready if we have a splice indicator in the reestablish.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Landing as its mostly comment changes and one simple change.
4 participants
The requirements for the
next_fundingTLV inchannel_reestablishwere changed in lightning/bolts#1289. The corresponding changes were in #3886, but that PR predated the spec changes. This PR updates the implementation to reflect the spec PR.